Keep fast typing from being silently dropped by MaggieAppleton · Pull Request #358 · githubnext/chopin

MaggieAppleton · 2026-10-07T23:22:13Z

Why

In the Decisions audit, a user typed four lines quickly. The last part ("d time to first edit.") stayed on their screen but never reached the other person, and it was gone after a reload. They got no notice. I reproduced this every time with the same Playwright typing against a local server.

The cause is the server's per-socket rate limit, not a Lexical or Yjs race. submit() in apps/server/src/plan/service.ts takes at most 200 updates a second from one socket and drops the rest without a reply. The provider sent one update per Lexical update. It only resent unacknowledged updates on the next plan:open, so a dropped update stayed in the outbox until a reconnect.

It's worse than losing the burst's tail. Yjs holds back any update whose predecessor never arrived. So after one drop, nothing else that client typed in the session reached anyone. Typing " later" slowly, 2.5 s after a 260-character burst, never showed up for the peer, and it was missing after a reload. The server's log showed seq stopping at exactly 2 + 200.

What changed

All changes are in packages/editor/src/provider.ts:

Paced sending. The first update goes out at once. Updates made in the next 10 ms are merged and sent as one message. That caps a client at about 100 messages a second, half the server's limit.
Resending. If an update is still unacknowledged after a full resend interval, it's sent again, merged. The interval starts at 2 s, doubles up to 30 s, and goes back to 2 s when an acknowledgement arrives. After five sends the provider stops and leaves the update for the next open to replay. Yjs updates are idempotent, so resending a slow update costs only a duplicate.
No repeats of refused updates. The provider stops resending when the server refuses rather than drops: implementation is active, or a same-epoch plan:reset for an update over the size limit. The next open still replays them.
Bounded merges. A replayed backlog and the existing saturation coalescing now merge into chunks under 64 KB. Before, they produced one update, and a large backlog could pass the server's 512 KB per-update limit and force a reset.

Provider flow

The provider groups rapid edits and retries an unacknowledged update up to five sends before waiting for the next document open.

Testing

bun test packages/editor/src/provider.test.ts: 19 pass. The new tests use a fake server that applies, acknowledges and drops updates. They cover:
- a 300-update burst against a 200 cap
- a dropped first update followed by a stranded second one
- no resend once an update is acknowledged
- backoff and the five-send cap
- no repeats after implementation is active or a same-epoch reset
- an epoch reset cancelling a pending resend
- late acknowledgements for ids that were merged away being ignored
- a 256 KB replay split into chunks of 64 KB or less
The burst and stranded-update tests failed before the fix.
bun test packages/editor apps/web: 988 pass on the first commit. bun test packages/editor: 491 pass after the follow-up.
bun run types: clean. bun run ci: passes. bun run fix: no changes to these files.
New e2e/collab.e2e.ts test: Playwright types a burst with no delay, then slow text. It checks the peer and the durable source. Final rebased head f7cd0667 passed hosted validation, E2E (including the burst-and-slow peer test), and container checks.
Manual check against a fake-GitHub server with two browser contexts. Before: the burst-then-slow script showed the peer 201 of 261 characters and no " later", and a reload matched the peer. After: 261, " later" present, and the reload matched. The original audit typing script now converges for the peer and after a reload. Concurrent typing by two people with 0 to 6 ms key delays converged in all 8 runs, both before and after.

Not changed (findings)

"Remote update and keystroke in the same tick." This is real at the Lexical level. syncYjsChangesToLexical uses a non-discrete update tagged collaboration, and syncLexicalUpdateToYjs skips any update with that tag. So a local edit merged into the same pending state is never sent. A headless harness reproduces it when both happen synchronously. In a browser, keystrokes and socket messages are separate tasks, and Lexical commits each in a microtask. I found no path where they share a pending state. Every keystroke update I instrumented in the browser had no tags.
Duplicate trailing paragraphs. Every client does append its own trailing paragraph after a card the server puts at the end. room.apply accepts those batches, and empty paragraphs don't serialize. This can't cause protected-projection.
A rejected batch still drops innocent edits silently. rejectBatch rebuilds the room and broadcasts plan:reset. Each client clears its outbox and remounts, so unacknowledged edits from everyone in that 5 ms batch are lost. PlanEditor stores state.reset but never shows it. That needs a design decision, so it's left for a follow-up.

🤖 Generated with Claude Code

MaggieAppleton · 2026-10-07T23:36:56Z

Hold — please don't merge yet. This changes collaborative sync correctness and an adversarial review of the latest commit is still in progress. I'll post the verdict here.

🤖 Generated with Claude Code

MaggieAppleton · 2026-10-07T23:45:25Z

Review: Looks good

I checked the code adversarially against AGENTS.md (Runtime model, Collaboration failure traps). bun test packages/editor/src/provider.test.ts passes 14 of 14. bun test packages/editor apps/web passes 987 of 988 locally. The one failure is icon-assets.test.ts hitting its 5 s timeout on a slow local run. It's unrelated and passes in CI. CI is green: validation, e2e, and container. There are no UI changes, so I skipped the visual check.

Correctness: holds up

Ack before removal. An entry leaves #outbox only through #settle (an ack) or by being merged into a new entry, and that new entry is itself held until it is acked. The rule that a draft edit must not leave the queue until it's acknowledged still holds.
Merging is safe. Y.mergeUpdates over one client's updates, including ones with gaps where acked entries already left, still produces one valid update. Late acks for the replaced ids are ignored. The merged id is settled only by its own ack, so the worst case is a duplicate, and Yjs updates are idempotent.
Epoch changes discard correctly. #reset and a rotated #open clear #unsent, #overdue and the timers. submit() also ignores updates sent with a stale epoch. Same-epoch reconnects replay the outbox merged, in chunks under 64 KB, well below LIMITS.update.
Offline is fine. wire.send drops frames when the socket isn't open, so resends while offline don't pile up. The next plan:open replays.
Pacing is fine. The first update goes out at once, and later ones wait at most 10 ms, which peers won't notice. That's about 100 messages a second per socket plus occasional resends, comfortably under 200.

Should the server change too? Not in this PR, but please open a follow-up
submit() at apps/server/src/plan/service.ts still does if (gauge.recent.length >= RATE_LIMIT) return; with no reply and no log. With this PR, current clients stay under the limit, and if an update is dropped anyway they recover in 2 to 4 s. A nack wouldn't help older or custom clients, because they ignore replies to plan:update. That includes tabs still running the old bundle after a deploy. Only something that makes them reconnect and replay would help them. Suggested follow-up:

Log a warning once per rate-limit window when an update is dropped, so this is visible in diagnostics. Today it fails with nothing in the server log.
Reply with fail(ws, msg.rid, "rate limited"). It's cheap and makes the drop visible on the wire.
Optionally, close the plan subscription or socket after sustained excess. That forces old clients to replay their outbox instead of being stranded.

Minor

provider.ts #resend: if the server never acks for a reason other than a drop, the provider resends every 2 to 4 s indefinitely. Two examples: an update over 512 KB gets a same-epoch plan:reset that #reset ignores, and implementation is active gets a fail reply. Both cases already existed and the editor is normally locked during them. Before, the update was sent once; now it's sent forever. Consider stopping or backing off after a few tries.
If commits are slow (over 2 s, e.g. PostgreSQL latency), every connected client resends its unacked backlog. It's bounded by the 64 KB chunks and harmless, but it adds load when the server is already struggling. Doubling RESEND_MS on each consecutive resend would soften that.
Test gaps in provider.test.ts: there's no unit test that a plan:reset mid-backlog cancels the pending resend, that an ack for a pre-merge id is ignored while the merged id stays outstanding, or that a large replay is split into chunks under MERGE_BYTES. These are each a few lines with the new server() fake.
The PR description cites apps/web/src/collab.test.ts with real providers against a real server. I couldn't find that file on this branch. Please correct the Testing section.

MaggieAppleton · 2026-10-08T00:22:44Z

Thanks for the review. I addressed it in bbbbc16, and CI is green (validation, e2e, container).

Backoff. The resend interval starts at 2 s, doubles up to 30 s, and goes back to 2 s when any acknowledgement arrives.
Send cap. After five sends without an acknowledgement, an update is left for the next open to replay.
Refusals stop resends. A session:error saying implementation is active, or a same-epoch plan:reset (the server's answer to an update over 512 KB), stops resends of what's outstanding. Neither refusal can be matched to an update id: wire.send sets its own rid, and the oversize reset carries none. So all outstanding updates are held for the next open, which replays them.
New tests, now 19 in provider.test.ts:
- backoff timing and the five-send cap
- no repeats after either refusal
- an epoch reset cancels a pending resend
- late acknowledgements for ids that were merged away are ignored, and the merged id stays outstanding until it's acknowledged
- a 256 KB replay goes out in chunks of 64 KB or less
PR description. The Testing section is fixed: apps/web/src/collab.test.ts doesn't exist.
Server follow-up. That's Make the plan update rate limit visible #373. A dropped update now gets a rate limited reply, there's one warning per window, and the socket closes with code 4429 after three windows in a row, so older clients reconnect and replay.

MaggieAppleton · 2026-10-08T00:22:46Z

The hold is lifted. The review points are addressed in bbbbc16, and CI is green.

The plan service accepts at most 200 updates a second from one socket and ignores the rest without replying. The provider sent one update per Lexical update and only resent unacknowledged ones on the next open, so a fast burst lost its tail, and Yjs then held every later update from that client behind the missing one: nothing else typed in that session reached peers or survived a reload. The provider now merges updates made within 10 ms into one message, resends anything still unacknowledged after a full resend interval, and merges a replayed backlog into chunks under 64 KB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Resends now double from 2 s up to 30 s, reset when an acknowledgement arrives, and stop after five sends. An update the server refused outright (implementation active, or an oversized update answered with a same-epoch reset) is not repeated; the next open still replays it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

MaggieAppleton mentioned this pull request Oct 8, 2026

Make the plan update rate limit visible #373

Merged

MaggieAppleton and others added 2 commits October 8, 2026 05:17

MaggieAppleton force-pushed the design/concurrent-keystroke-loss branch from bbbbc16 to f7cd066 Compare October 8, 2026 04:18

MaggieAppleton merged commit c78d327 into main Oct 8, 2026
3 checks passed

MaggieAppleton deleted the design/concurrent-keystroke-loss branch October 8, 2026 04:28

Navigation Menu

Repository navigation

Keep fast typing from being silently dropped - #358

Keep fast typing from being silently dropped#358
MaggieAppleton merged 2 commits into
mainfrom
design/concurrent-keystroke-loss

MaggieAppleton commented Oct 7, 2026 •

edited

Loading

Uh oh!

MaggieAppleton commented Oct 7, 2026

Uh oh!

MaggieAppleton commented Oct 7, 2026

Uh oh!

MaggieAppleton commented Oct 8, 2026

Uh oh!

MaggieAppleton commented Oct 8, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MaggieAppleton commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Provider flow

Testing

Not changed (findings)

Uh oh!

MaggieAppleton commented Oct 7, 2026

Uh oh!

MaggieAppleton commented Oct 7, 2026

Uh oh!

MaggieAppleton commented Oct 8, 2026

Uh oh!

MaggieAppleton commented Oct 8, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

MaggieAppleton commented Oct 7, 2026 •

edited

Loading